-
Notifications
You must be signed in to change notification settings - Fork 47
Introduction of field API interface in ectrans #314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Without this a test with an incorrect argument still passes.
…into dump-checksums
| INTEGER(KIND=JPIM) :: IFLDXGUV | ||
| INTEGER(KIND=JPIM) :: IFLDXLUV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| INTEGER(KIND=JPIM) :: IFLDXGUV | |
| INTEGER(KIND=JPIM) :: IFLDXLUV |
These aren't used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in ecbf2d2
| IFLDXGUV= 0 | ||
| IFLDXLUV= 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. they will go away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in ecbf2d2
| ! ------------------------------------------------------------------ | ||
| IF (LHOOK) CALL DR_HOOK('INV_TRANS_FIELD_API',0,ZHOOK_HANDLE) | ||
|
|
||
| ISPUV = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ISPUV also doesn't seem to be used - can you check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct. I am removing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 7cc7912
| ! Spectral field view | ||
| REAL(KIND=JPRB),POINTER :: P(:) | ||
| INTEGER :: IVSET | ||
| CHARACTER(LEN=12) :: NAME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find a case where NAME is used. Could you check this is still required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Names are really useful for debugging, I would prefer to keep them
| ! Grid point field view | ||
| REAL(KIND=JPRB),POINTER :: P(:,:) | ||
| INTEGER :: IVSET | ||
| CHARACTER(LEN=12) :: NAME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
|
I took the liberty to commit directly in your branch @dhaumont, I hope you don't mind. |
|
It is not possible that a downstream package would be linking to multiple versions of ectrans_field_api (i.e. ectrans_field_api_cpu_sp, ectrans_field_api_cpu_dp, ectrans_field_api_gpu_sp, ectrans_field_api_gpu_dp). However this means that we don't need to go through the same procedure as done with trans to define unique symbols, as the uniqueness is not important. For trans this was important because we want to be able to make use of trans through downstream libraries (like Atlas) that may make a runtime choice on precision and cpu/gpu execution. If you don't mind I will refactor this a bit, reducing the complexity also with the includes which would not be required any longer. |
So the field api interface is not compiled nor executed in the CI ???? |
One of the main requirement of this interface is the ability to dynamically choose betweeen CPU and GPU execution: you want to be able to run the spectral transform on CPU or on GPU with LDACC. I'm not sure what problem you are trying to solve exactly, is this the fact that you have to include an interface file ? |
It's tested in the "HPC" suite (ECMWF HPC2020 and LUMI) but not the Github suite. So what we need is a commit like 88af96b but applied to .github/workflows/build.yml. |
The FIELD_API feature is optional, meaning that when FIELD_API is not enabled those particular tests should not be running, we can all agree on that. That is what I fixed. I forgot to mention that for the build-hpc workflow I also added the missing field_api dependency in commit ce5ae51 . It was not sufficient to set "-DENABLE_FIELD_API=ON", so hopefully the CI is going to test this at least on the HPCs now (lumi and ecmwf-atos). |
OK, thanks a lot Willem. |
I think the simplicification will still allow us to develop later on the ability to choose between CPU and GPU backends, but not between single and double precision, due to linking to two field_api libraries that are incompatible with each other, i.e. the meanings of FIELD_1RB, FIELD_2RB etc. What will be possible in the future is to have two libraries : It is however not yet clear to me if and then introspection on the KRESOL parameter will need to be used instead of the LDACC flag. What is clear to me however is that you also did not intend to create these symbols: |
|
See dhaumont#4 for the proposed changes to remove the unique symbol handling with the sedrenames etc. |
Remove unique symbols for ectrans_field_api
|
@samhatfield @wdeconinck After our discussion about different passes in LS and LG that were not needed anymore, I simplified the code by making the rguments optional. I also removed the passes. IMPORTANT: I just found a crash in the unit test due to this commit. I'm inverstigating |
Summary
This PR introduces a field API interface to ectrans. It is based on top of dump-checksums (PR #287)
Description of changes
The new field API interface is implemented in
src/field_api:src/field_api/trans/inv_trans_field_api.F90: inverse transform (calling inv_trans internally)src/field_api/trans/dir_trans_field_api.F90: direct transform (calling dir_trans internally)src/field_api/field_api_ectrans_mod.F90: helper functionsOptional compilation triggered by a new USE_FIELD_API configuration option. Adds a new dependency to field API.
Single and double precision for CPU and GPU
Accessible in ectrans-benchmark via a new option
--field-apisrc/programs/util/ectrans_field_api_helper.F90contains the field API implementationIn the GPU version of ectrans-benchmark , setting
llacc = .true.will force the intermediate fields on GPU. This requires additional memory copies host <-> device before and after the regular ectrans calls.New unit tests have been added to test
--field-api(CPU and GPU)compare_checksums.pyupdated to compare the checksums withfield-apiwith the regular versionnvhpc22.11 on CPU: bit reproducibility between the field API and the regular version has been partially adressed by clamping small values in spectral space after dir_trans in ectrans benchmark (2a7aa87). This only applies to double precision (single precision is only bit reproducible during the first time step and slightly different after)
Testing and validation
*Gfortran 13.3.0 - CPU : bit reproducible
Known limitations
llacc = .true.requires memory transfers between CPU and GPU, that should be avoidedInitial pull request
dhaumont#1
Contains some comments from @samhatfield that needs to be integrated
Prototype in ARPEGE
See for instance this prototype of transinv_mdl_field_api.F90: https://github.com/dhaumont/IAL/blob/50t1_field_api_ectrans/arpifs/transform/transinv_mdl_field_api.F90
This version can be compiled using this bundle:
50t1_ectrans_field_api_interface